cache: fix cache proto's max_body_bytes type by enozcan · Pull Request #12273 · envoyproxy/envoy

enozcan · 2020-07-24T11:09:16Z

Commit Message: Change max_body_bytes type in cache.proto to uint64.

Additional Description: uint64_t is used for response body sizes in cache filter (AdjustedByteRange, LookupResult::content_length_ etc.) where max_body_bytes field is uint32 in cache.proto. Related discussion with @toddmgreer : #10536 (comment)

I also wonder how to change (or generate) these fields under generated_api_shadow as it says: Do not hand edit any file under envoy/ and as I see they are not generated after build.

Risk Level: No usage of this field exists.
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

repokitteh-read-only · 2020-07-24T11:09:24Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12273 was opened by enozcan.

see: more, trace.

zuercher · 2020-07-24T22:07:42Z

You can regenerate the shadow API files by running tools/proto/proto_format.sh.

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

alyssawilk · 2020-07-27T13:06:14Z

Sorry, we've been having some CI issues lately. Can you try a master merge to see if it sorts out CI unhappiness?

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

stale · 2020-08-08T10:29:41Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

jmarantz

/wait

jmarantz · 2020-08-15T17:56:31Z

api/envoy/config/filter/http/cache/v2alpha/cache.proto

  // Max body size the cache filter will insert into a cache. 0 means unlimited (though the cache
  // storage implementation may have its own limit beyond which it will reject insertions).
-  uint32 max_body_bytes = 4;
+  uint64 max_body_bytes = 4;


you can't change the v2 protos now.

jmarantz · 2020-08-15T17:56:54Z

api/envoy/extensions/filters/http/cache/v3alpha/cache.proto

  // Max body size the cache filter will insert into a cache. 0 means unlimited (though the cache
  // storage implementation may have its own limit beyond which it will reject insertions).
-  uint32 max_body_bytes = 4;
+  uint64 max_body_bytes = 4;


I'm wondering if we need to deprecate this field and make a new one with the different type. @htuch ?

I think the question is do we realistically expect to have cache objects > 4GB? If yes, then let's do this new/old deprecation dance, otherwise let's handle the type mismatch in the implementation.

@toddmgreer Should we commit to 4G limit in cacheable object sizes? Seems OK to me.

Maybe in practicality, as there are no real impls of this filter yet, it doesn't matter if we change the type as there's no outstanding data to be compatible with.

I don't know why anyone would want to set a size limit that's higher than 4G, but I have no objection to allowing it. I also have no objection to leaving it as is.

toddmgreer · 2020-08-18T01:28:55Z

https://developers.google.com/protocol-buffers/docs/proto#updating says:

int32, uint32, int64, uint64, and bool are all compatible – this means you can change a field from one of these types to another without breaking forwards- or backwards-compatibility. If a number is parsed from the wire which doesn't fit in the corresponding type, you will get the same effect as if you had cast the number to that type in C++ (e.g. if a 64-bit number is read as an int32, it will be truncated to 32 bits).

I think the question is thus whether anyone will send a v3+ configuration with max_body_bytes > 4GB to a deployment that expects a v2 configuration. There aren't any deployments of this alpha filter, so that isn't going to happen.

…

On Mon, Aug 17, 2020 at 5:51 PM htuch ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In api/envoy/extensions/filters/http/cache/v3alpha/cache.proto <#12273 (comment)>: > @@ -80,5 +80,5 @@ message CacheConfig { // // Max body size the cache filter will insert into a cache. 0 means unlimited (though the cache // storage implementation may have its own limit beyond which it will reject insertions). - uint32 max_body_bytes = 4; + uint64 max_body_bytes = 4; I think the question is do we realistically expect to have cache objects > 4GB? If yes, then let's do this new/old deprecation dance, otherwise let's handle the type mismatch in the implementation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12273 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRAWPK5POIU2JHGVZNSZVDSBHGAHANCNFSM4PGTSN2Q> .

jmarantz

Sorry -- we still need to back out the v2 change.

Then @htuch needs to approve for the API change.

Not passing CI, as you can't change v2 anymore.

htuch · 2020-08-18T18:00:26Z

We're actually stricter than wire compatibility and need to ensure we don't break language bindings as well, see https://github.com/envoyproxy/envoy/blob/master/api/API_VERSIONING.md#backwards-compatibility.

toddmgreer · 2020-08-18T22:38:31Z

It says "Fields should not be renumbered or have their types changed. This is standard proto development procedure." However, this kind of type change (uint32->uint64) is perfectly fine in standard proto development procedure. I think the rule is overly conservative, and that this change doesn't break language bindings. Specifically, I think widening should be allowed.

…

On Tue, Aug 18, 2020 at 11:00 AM htuch ***@***.***> wrote: We're actually stricter than wire compatibility and need to ensure we don't break language bindings as well, see https://github.com/envoyproxy/envoy/blob/master/api/API_VERSIONING.md#backwards-compatibility . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12273 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRAWPIDXIVXCPDY6QJGJMLSBK6UXANCNFSM4PGTSN2Q> .

htuch · 2020-08-19T00:02:17Z

Possibly, @envoyproxy/api-shepherds WDYT? I'd ask for you to update the style guide at the same time if we do this.

mattklein123 · 2020-08-19T00:05:26Z

Fine with me!

stale · 2020-08-29T13:21:21Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

enozcan · 2020-09-01T09:52:36Z

@jmarantz Reverted v2 protos and I wonder if there should be any further changes - docs, deprecations, etc. regarding the conversations above.

stale · 2020-09-11T07:41:32Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz

I lost track of this PRl. Is this still active?

jmarantz · 2020-09-18T11:10:11Z

api/envoy/extensions/filters/http/cache/v3alpha/cache.proto

  // Max body size the cache filter will insert into a cache. 0 means unlimited (though the cache
  // storage implementation may have its own limit beyond which it will reject insertions).
-  uint32 max_body_bytes = 4;
+  uint64 max_body_bytes = 4;


@toddmgreer Should we commit to 4G limit in cacheable object sizes? Seems OK to me.

Maybe in practicality, as there are no real impls of this filter yet, it doesn't matter if we change the type as there's no outstanding data to be compatible with.

jmarantz · 2020-12-22T13:43:22Z

/wait

github-actions · 2021-02-22T20:02:52Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-03-02T00:05:50Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Change max_body_bytes type to uint64.

b6d9b66

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

repokitteh-read-only bot added v2-freeze api labels Jul 24, 2020

toddmgreer previously approved these changes Jul 24, 2020

View reviewed changes

Generate shadow api.

e04f9c6

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

enozcan dismissed toddmgreer’s stale review via e04f9c6 July 24, 2020 22:39

alyssawilk assigned jmarantz Jul 27, 2020

Merge branch 'master' into config-type-fix

86fcb3d

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

mattklein123 added the waiting label Jul 30, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 8, 2020

enozcan added 2 commits August 10, 2020 10:27

Merge branch 'master' into config-type-fix

29735fe

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

Run proto_format after master merge.

9f8f327

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

repokitteh-read-only bot removed the waiting label Aug 10, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 10, 2020

jmarantz reviewed Aug 15, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 15, 2020

jmarantz previously approved these changes Aug 18, 2020

View reviewed changes

jmarantz reviewed Aug 18, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 29, 2020

enozcan added 2 commits September 1, 2020 10:39

Merge branch 'master' into config-type-fix

0346432

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

Revert v2 changes.

c86bc41

Signed-off-by: Enes Ozcan <enes.ozcan@hazelcast.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Sep 1, 2020

repokitteh-read-only bot removed the waiting label Sep 1, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 11, 2020

mattklein123 removed the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020

jmarantz reviewed Dec 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 22, 2020

Base automatically changed from master to main January 15, 2021 23:01

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 22, 2021

github-actions bot closed this Mar 2, 2021

Conversation

enozcan commented Jul 24, 2020

Uh oh!

repokitteh-read-only bot commented Jul 24, 2020

Uh oh!

zuercher commented Jul 24, 2020

Uh oh!

alyssawilk commented Jul 27, 2020

Uh oh!

stale bot commented Aug 8, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

jmarantz Aug 15, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

jmarantz Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

toddmgreer Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

toddmgreer commented Aug 18, 2020 via email

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 18, 2020

Uh oh!

toddmgreer commented Aug 18, 2020 via email

Uh oh!

htuch commented Aug 19, 2020

Uh oh!

mattklein123 commented Aug 19, 2020

Uh oh!

stale bot commented Aug 29, 2020

Uh oh!

enozcan commented Sep 1, 2020

Uh oh!

stale bot commented Sep 11, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Dec 22, 2020

Uh oh!

github-actions bot commented Feb 22, 2021

Uh oh!

github-actions bot commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants